Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference reactor-ts as module #1322

Merged
merged 25 commits into from
Aug 30, 2022
Merged

Reference reactor-ts as module #1322

merged 25 commits into from
Aug 30, 2022

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Aug 9, 2022

This PR changes the build process to, rather than compile generated code in conjunction with the reactor-ts code, compile separately and import reactor-ts as a module. In principle, this flow would enable building reactor-ts just once and reuse it for all tests, but we currently don't do this (yet). Once reactor-ts stabilizes and has regular releases, it should suffice to treat is as a normal dependency and pull it from NPM like all other dependencies. Then, pnpm can take care of caching, which should speed up build times considerably.

@petervdonovan
Copy link
Collaborator

petervdonovan commented Aug 22, 2022

Such issues as these may delay progress for some days.

This change is made out of pure laziness. Human time is worth more
than computer time -- and disk space.
@petervdonovan
Copy link
Collaborator

I just ran into an issue where the generated directory for Minimal.lf consumes over 300 MB 😅 which is why we ran out of disk space on Windows in CI. Even in master it uses 130 MB, so instead of reverting I'll try to find a fix.

@petervdonovan petervdonovan force-pushed the reactor-ts-npm branch 3 times, most recently from 6f966e7 to 58a7ed0 Compare August 26, 2022 23:54
@petervdonovan
Copy link
Collaborator

petervdonovan commented Aug 28, 2022

Okay, I followed through with what I talked about with Marten (not bothering to pre-bundle the runtime or to install its dependencies) and the tests seem to be passing now, except for the LSP tests. I will try to get this done this weekend, although I might be sidetracked/delayed by the fact that the language server vscode extension no longer builds for me.

There is no need to use resolve because we are copying from class
path, where paths are standardized to use / as a separator.
Copy link
Member Author

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor comments...

org.lflang/src/lib/ts/package.json Show resolved Hide resolved
@petervdonovan
Copy link
Collaborator

Here is the result of running npx depcheck in the src-gen folder for Minimal.lf:

Unused dependencies
* @babel/node
* @babel/plugin-proposal-class-properties
* @babel/plugin-proposal-object-rest-spread
* @babel/plugin-proposal-optional-chaining
* @babel/plugin-transform-modules-commonjs
* @babel/preset-env
* @babel/preset-typescript
Unused devDependencies
* @types/google-protobuf
* @types/node
* @types/uuid
* ts-protoc-gen

The dev dependencies are not even installed by our code generator, I believe, so they should not matter for compile times. I do not require any dependency removals to be low-hanging fruit. Any protobuf-related dependency removal will require us to edit the package.json file conditionally in the code generator, which will add complexity to it. As for the build system -- I personally have had difficulty with the build system and prefer not to tinker with it further right now.

Is there any reason not to merge this as-is?

@lhstrh lhstrh merged commit 52905e0 into master Aug 30, 2022
@lhstrh lhstrh added the refactoring Code quality enhancement label Aug 30, 2022
@petervdonovan petervdonovan mentioned this pull request Oct 22, 2022
3 tasks
@cmnrd cmnrd deleted the reactor-ts-npm branch March 10, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants